Closed Bug 330397 Opened 18 years ago Closed 18 years ago

Additional problems with incomplete cache entries and cache flush assertions.

Categories

(Core :: Networking: HTTP, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: darin.moz, Assigned: darin.moz)

References

Details

Attachments

(1 file, 1 obsolete file)

Additional problems with incomplete cache entries and cache flush assertions.

This bug is basically about follow-up for my patch for bug 189570.
Status: NEW → ASSIGNED
Depends on: 329260
Target Milestone: --- → mozilla1.9alpha
Attached patch v1 patch (obsolete) — Splinter Review
This patch cleans up a bunch of stuff.  The logic for dooming a cache entry now widdles down to whether or not we have initialized it.  If we receive READ+WRITE access to the cache entry, then we can assume that it was previously initialized.  If we only have WRITE access, then we were responsible for initializing it.  The mOpenedCacheForWriting flag is then replaced with a mInitedCacheEntry flag, that is set if InitCacheEntry succeeds.
Attachment #214950 - Flags: superreview?(bzbarsky)
Attachment #214950 - Flags: review?(cbiesinger)
Oh, the change to nsDiskCacheStreams.cpp is to avoid an assertion resulting from trying to flush a doomed entry.  Doomed entries are no longer present in the _CACHE_MAP_, so it makes no sense to try to flush their data to disk.
Comment on attachment 214950 [details] [diff] [review]
v1 patch

sr=bzbarsky if us no longer checking the IsResumable() thing is ok.
Attachment #214950 - Flags: superreview?(bzbarsky) → superreview+
Boris, we check for that inside CheckCache.
Ah, ok.  ;)
Attachment #214950 - Flags: review?(cbiesinger) → review+
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
With this fix included, I am unable to download executable files using firefox.  The file appears to be truncated as if the final buffer was not flushed.  Backing out this patch corrects the issue.  There are many complaints about this issue using the Firefox trunk 3/22 nightly on the Mozillazine forums.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hmm... ah, I suspect there may be some 'cache as file' consumers who expect to be able to doom the cache entry and still access the cache file while holding onto the cache entry descriptor.  Yeah, that could explain the regression.  In that case, I think I'll need to simply suppress editing the cache map when the entry is doomed.  OK, backing out the v1 patch for now.
Status: REOPENED → ASSIGNED
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060322 Firefox/1.6a1 ID:2006032301

first time to download an exe file is failed.
but second time to download the same file is complete.

this seems to be related to cache, but why the first time is failed.

now this checkin is backed out, so 0323 build is almost same with 0321 build (before this checkin landed.), why can not download at first time ?
Hmm, it appears I was wrong in comment #7.  I stupidly forgot to clear the cache before redoing my test downloads after backing out the patch from my tree. So, it would appear that it is a different checkin that caused the download regression.  Unfortunately, none of them really jump out as looking likely.
Perhaps it is really caused by bug 162361?  Unfortunately I don't have time to do a build right now.
OK. I decided I did have time to do a build.  I reapplied this patch and backed out 162361.  Then I removed the truncated files and cleared the cache and was able to successfully download my test files on the fist try.

Sorry for my previous misinformation. :-(
Forgot to mention, I filed bug 331453 on the download issue, and made it block 162361.  Sorry for the bugspam.
Priority: -- → P1
I modified the cache changes slightly to account for the possibility of a doomed cache entry that is being 'streamed as file'
Attachment #214950 - Attachment is obsolete: true
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
Depends on: 339459
Depends on: 335909
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: